Ce/connect messaging #35364

calellowitz · 2024-11-14T03:35:13Z

Product Description

This adds connect messaging as a new message type in the messaging framework. It does not currently support form sessions although I will add those prior to merge. That said, I wanted to open this up to review now since it is quite large and touches a fair amount of complex code.

Technical Summary

Tech Spec: https://docs.google.com/document/d/1FgsXr0PLR7Btq2fOBeXIMs_OziBRsUsATgHgWna0h5M/edit?tab=t.0#heading=h.3rzu21agk2m3
Design Doc: https://docs.google.com/document/d/1h3bcZ4oQAnOu5aGW8Rue0TnbiaW2OXWMaqNKwI6RciE/edit?tab=t.0#heading=h.ipgou4bu0qoj

The linked docs provide much of the context

Feature Flag

COMMCARE_CONNECT for now, but I expect to migrate it to its own flag so that it can be enabled independently of the rest of the connect features.

Safety Assurance

Safety story

Existing messaging paths have automated coverage, and are only minimally modified. The new code is almost entirely self contained and will go through a QA process.

Automated test coverage

QA Plan

No QA ticket yet since it is not finished, but I will add it once it starts.

Migrations

The migrations in this code can be safely applied first independently of the code

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

corehq/apps/sms/api.py

corehq/apps/sms/migrations/0061_connectmessage_message_id_connectmessage_received_on.py

corehq/apps/sms/models.py

corehq/messaging/scheduling/forms.py

orangejenny · 2024-11-18T20:45:36Z

corehq/messaging/scheduling/forms.py

@@ -1757,6 +1770,10 @@ def create_form_helper():
    def form_choices(self):
        return [(form['code'], form['name']) for form in get_form_list(self.domain)]

+    @property
+    def can_use_connect(self):


Nitpick: is this better than just having the calling code check the feature flag?

orangejenny · 2024-11-18T20:46:33Z

corehq/apps/domain/auth.py

@@ -405,3 +407,19 @@ def user_can_access_domain_specific_pages(request):
        return False

    return couch_user.is_member_of(project) or (couch_user.is_superuser and not project.restrict_superusers)
+
+
+def connectid_token_auth(view_func):


It's weird that connectid is one word. I do see that it's consistent.

I think of connectid as its own thing (its a separate repo and site than connect), using one word feels like it implies that, rather than the id for connect, but I am not wedded to it.

connectid and "the id for connect` being two separate concepts sounds confusing, but I don't think I have enough context on the project to be certain or to make a better suggestion.

orangejenny · 2024-11-18T20:55:20Z

.../messaging/scheduling/migrations/0030_remove_connectmessagesurveycontent_message_and_more.py

            preserve_default=False,
        ),
        migrations.AddField(
            model_name='connectmessagesurveycontent',
            name='form_unique_id',
-            field=models.CharField(default=1, max_length=126),
+            field=models.CharField(default="1", max_length=126),


Why not just allow this to be blank? Or does 1 have a meaning?

Nope, this will be unnecessary once we combine the migrations as you mentioned above, we just needed any value to have a valid migration, even though that table was empty when I made this one, so it is never used.

snopoke

In general this looks good to me but I'm not able to fully review all the implications of the changes.

pxwxnvermx · 2024-11-27T07:55:08Z

corehq/apps/smsforms/tasks.py

@@ -76,8 +76,8 @@ def send_first_message(domain, recipient, phone_entry_or_number, session, respon
            messaging_subevent_id=logged_subevent.pk
        )

-        if isinstance(phone_entry_or_number, PhoneNumber):
-            send_sms_to_verified_number(
+        if isinstance(phone_entry_or_number, PhoneNumber) or isinstance(phone_entry_or_number, ConnectMessagingNumber):


nit: this can be simplified as isinstance supports giving it an iterable of types to match.

Suggested change

if isinstance(phone_entry_or_number, PhoneNumber) or isinstance(phone_entry_or_number, ConnectMessagingNumber):

if isinstance(phone_entry_or_number, (PhoneNumber, ConnectMessagingNumber)):

pxwxnvermx · 2024-11-27T07:57:08Z

corehq/messaging/scheduling/forms.py

@@ -1565,6 +1576,8 @@ def add_initial_for_content(self, initial):
            initial['content'] = self.CONTENT_SMS_CALLBACK
        elif isinstance(content, FCMNotificationContent):
            initial['content'] = self.CONTENT_FCM_NOTIFICATION
+        elif isinstance(content, ConnectMessageContent):
+            initial['conent'] = self.CONTENT_CONNECT_MESSAGE


nit: typo 😢

pxwxnvermx · 2024-11-27T07:59:03Z

corehq/messaging/scheduling/models/abstract.py

                result |= set(content.message)
-            elif isinstance(content, EmailContent):
+            elif isinstance(content, (EmailContent)):


Are the () braces around EmailContent required here?

corehq/apps/sms/handlers/fallback.py

corehq/apps/sms/views.py

orangejenny · 2024-12-19T13:34:21Z

corehq/apps/users/models.py


    class Meta:
        unique_together = ('domain', 'commcare_user')
+
+    def get_or_create_key(self):


This would be nice as a property that would silently get or create the key.

corehq/messaging/scheduling/models/content.py

corehq/apps/reports/standard/sms.py

orangejenny · 2024-12-19T14:06:49Z

Reviewed through the "update report" commit (c67ac9e)

orangejenny · 2024-12-19T14:07:25Z

Test failure looks real.

orangejenny · 2024-12-19T23:41:34Z

docker/hq-compose.yml

@@ -76,7 +76,7 @@ services:
      interval: 10s
      retries: 10
    volumes:
-      - ${VOLUME_PREFIX}${BLANK_IF_TESTS-postgresql:}/var/lib/postgresql/data
+      - ${VOLUME_PREFIX}${BLANK_IF_TESTS-postgresql:}/var/lib/postgresql14/data


accidental?

calellowitz requested review from snopoke and pxwxnvermx November 14, 2024 03:35

calellowitz requested review from esoergel, orangejenny and mkangia as code owners November 14, 2024 03:35

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Nov 14, 2024

calellowitz added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Nov 14, 2024

calellowitz force-pushed the ce/connect-messaging branch 2 times, most recently from 6a953fa to f22c90d Compare November 14, 2024 21:06

calellowitz added 7 commits November 14, 2024 16:09

add models and rename function

0043322

add connect backend

7bc3854

add connectid settings and urls

c609f0d

add connect messages to scheduling forms

e4b6deb

add necessary auth changes

929204d

update domain deletion code for new models

79154f0

use updated method name

460c794

calellowitz force-pushed the ce/connect-messaging branch from f22c90d to 460c794 Compare November 14, 2024 21:09

calellowitz added 2 commits November 14, 2024 22:24

implement survey model

092dbe0

fix default values

f29ebfc

orangejenny reviewed Nov 18, 2024

View reviewed changes

snopoke reviewed Nov 19, 2024

View reviewed changes

calellowitz added 4 commits November 22, 2024 15:44

move content fields to top level of payload

91ce6e6

use string for the key

4b992c7

return the value

36a800f

typo

472609a

pxwxnvermx reviewed Nov 27, 2024

View reviewed changes

calellowitz added 4 commits November 27, 2024 13:04

incoming messaging processing updates

1efd62e

handle connect content

80e0345

add create channel views

f327d55

add connect survey content

6d9e562

calellowitz added 4 commits December 18, 2024 19:07

use domain instead of domain_scope

0231864

fix function error

596eba4

pr feedback and report error

0fa683e

update report

c67ac9e

orangejenny reviewed Dec 19, 2024

View reviewed changes

corehq/apps/sms/handlers/fallback.py Show resolved Hide resolved

orangejenny reviewed Dec 19, 2024

View reviewed changes

corehq/apps/sms/views.py Outdated Show resolved Hide resolved

orangejenny reviewed Dec 19, 2024

View reviewed changes

corehq/apps/sms/views.py Outdated Show resolved Hide resolved

orangejenny reviewed Dec 19, 2024

View reviewed changes

corehq/messaging/scheduling/models/content.py Outdated Show resolved Hide resolved

orangejenny reviewed Dec 19, 2024

View reviewed changes

corehq/apps/reports/standard/sms.py Outdated Show resolved Hide resolved

calellowitz and others added 11 commits December 19, 2024 16:39

lint

10737b7

convert to f strings

8dd41f1

move copy and lint

a66ec40

move critical section to base class

c047e8e

make try block narrower

75184ce

add success message to channel creation

7d66924

replace get_sms_class

b652ddb

update sms docs

b28cdbf

Merge branch 'master' into ce/connect-messaging

867dd47

fix bad merge

f50e557

squash migrations

eccc425

orangejenny reviewed Dec 19, 2024

View reviewed changes

calellowitz added 3 commits December 19, 2024 18:50

replace method with property

ae2d380

add new models to domain deletion and dump

aaffbaa

add more models to domain deletion

7142730

calellowitz force-pushed the ce/connect-messaging branch from 6870aee to 7142730 Compare December 19, 2024 23:53

orangejenny approved these changes Dec 19, 2024

View reviewed changes

fix bug in channel creation and improve message

599af78

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ce/connect messaging #35364

Ce/connect messaging #35364

calellowitz commented Nov 14, 2024 •

edited

Loading

orangejenny Nov 18, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

orangejenny Nov 22, 2024

orangejenny Nov 18, 2024

calellowitz Nov 22, 2024

snopoke left a comment

pxwxnvermx Nov 27, 2024 •

edited

Loading

pxwxnvermx Nov 27, 2024

pxwxnvermx Nov 27, 2024

calellowitz Dec 19, 2024

orangejenny Dec 19, 2024

orangejenny commented Dec 19, 2024

orangejenny commented Dec 19, 2024

orangejenny Dec 19, 2024

	if isinstance(phone_entry_or_number, PhoneNumber) or isinstance(phone_entry_or_number, ConnectMessagingNumber):
	if isinstance(phone_entry_or_number, (PhoneNumber, ConnectMessagingNumber)):

Ce/connect messaging #35364

Are you sure you want to change the base?

Ce/connect messaging #35364

Conversation

calellowitz commented Nov 14, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snopoke left a comment

Choose a reason for hiding this comment

pxwxnvermx Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orangejenny commented Dec 19, 2024

orangejenny commented Dec 19, 2024

Choose a reason for hiding this comment

calellowitz commented Nov 14, 2024 •

edited

Loading

pxwxnvermx Nov 27, 2024 •

edited

Loading